Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite chruby-fish in fish #39

Merged
merged 19 commits into from
May 17, 2022
Merged

Rewrite chruby-fish in fish #39

merged 19 commits into from
May 17, 2022

Conversation

bouk
Copy link
Collaborator

@bouk bouk commented Feb 16, 2020

Hi, I rewrote chruby.fish and auto.fish from scratch, so it doesn't rely on the bash chruby. This makes things faster and fixes #31, closes #37, closes #36

I ran the tests and they pass, except for the one about sourcing .bash_profile of course, so I removed that one.

@bouk bouk requested a review from JeanMertz February 16, 2020 09:24
@bouk
Copy link
Collaborator Author

bouk commented Feb 17, 2020

I'm not going to try and keep debugging this travis mess!

@bouk
Copy link
Collaborator Author

bouk commented Feb 17, 2020

oh sweet it worked

Copy link
Owner

@JeanMertz JeanMertz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work @bouk and thanks for getting the tests to pass.

Interestingly enough, the original version (5869c05) of this library was actually written in pure Fish. It was later rewritten as a wrapper script in 3c94b5a to make it easier to keep up-to-date with the official chruby scripts.

Having said that, I think chruby itself has matured over the years, and there have been no breaking changes for several years now. Similarly, Fish has also matured a lot and has gained many built-in functions that make it easier and more performant to use a pure-Fish implementation.

So I'd say now would be a good time to switch back to a pure Fish implementation.

I left a few remarks, but overall this looks good.

I propose we release this as 1.0.0-beta.1 so that we can get some testing going, before we release the final 1.0.0 version which basically makes this library "done", other than bug fixes we might fix going forward.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
share/fish/vendor_functions.d/chruby.fish Show resolved Hide resolved
.travis.yml Outdated

env:
global:
- HOMEBREW_NO_AUTO_UPDATE=1
matrix:
- FISH_VERSION="--HEAD"
- FISH_VERSION="" # 2.5.0
- FISH_VERSION="" # 3.1.0
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably a good idea to add a note to the README about the minimum supported Fish version, given that we're now using some built-in functions that only became available in later versions of Fish.

I'm fine with setting the minimum supported version to the latest stable release of Fish. If others find that things work on their version, I'd accept PRs reducing the minimum supported version based on their feedback.

@@ -2,7 +2,7 @@ set -x PROJECT "$PWD"
set -x PREFIX "$PROJECT/test"
set -x HOME "$PREFIX/home"

source ./share/chruby/chruby.fish
set fish_function_path $PROJECT/share/fish/vendor_functions.d $fish_function_path
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want auto to always load in our tests. We want to test auto-loading separate from the regular setup, so that we know both behave as expected.

@bouk
Copy link
Collaborator Author

bouk commented Mar 16, 2020

I've addressed your comments @JeanMertz ! Thanks for the review

@JeanMertz
Copy link
Owner

@bouk thanks so much for the effort you put in.

I've been using the Fish version for a while since I've had to work on a Ruby project for the first time in a long while.

I'm going to keep this PR open for a while, but so far, things look promising 👍 I do notice some slowdowns in certain situations, but haven't had enough time to dig in yet. I suspect it has something to do with auto-detection, but I'm not sure yet.

@bouk
Copy link
Collaborator Author

bouk commented Apr 1, 2020

I'm very curious where it's slower! I wonder if you could try fish --profile to dig into it.

@waynehoover
Copy link

This is great, is there an easy way to test this now?

@monfresh
Copy link

Hello. Any updates on this? Is there anything we can do to allow this to be merged?

@lazyatom
Copy link

This is great, is there an easy way to test this now?

I'm testing it by:

  1. uninstalling the existing version (I installed it using homebrew, so brew uninstall chruby-fish),
  2. cloning this fork
  3. checking out the branch in this PR (rewrite-fish)
  4. running make install
  5. removing lines in my fish config that sourced the old version (e.g. source /usr/local/share/chruby/chruby.sh); this approach doesn't need them

@monfresh
Copy link

@JeanMertz Do you still have time to maintain this repo? If not, would it make sense to add other maintainers? It would be really great to have this PR merged and released. Thanks!

@bouk
Copy link
Collaborator Author

bouk commented Dec 21, 2020

I would gladly take over maintenance

@ioquatix
Copy link
Collaborator

Any update on this?

@ioquatix
Copy link
Collaborator

My made my own fork wit these changes and can confirm they work as expected.

@todd-a-jacobs
Copy link

In my personal experience, RVM is very Bourne-centric. As a result, it doesn't play well with Fish. One way around this would be to use RVM to install a given Ruby, but then call a clean environment for the actual chruby testing. I haven't spent much time on this yet, but it seems like leveraging RVM to create the environment and then using another stage or add-on to ensure that we're actually in Fish without RVM (but with chruby installed) would be useful.

After looking at the code, it looks like the Travis CI YAML is more or less doing that already. If that's the case, what's the actual problem that still needs to be solved here?

@ioquatix
Copy link
Collaborator

ioquatix commented Nov 24, 2021

@bouk @JeanMertz any update on where we are at with this?

I've been testing my fork on lots of machines and it is working well. https://github.com/ioquatix/chruby-fish

@tenderlove
Copy link
Contributor

Hi,

Sorry to be another noisy +1 on this PR, but I also ran in to #31 and this PR fixes it. My use case is that I'm on an M1 now, but I want to have a Terminal that opens under Rosetta and one that does not. Then have my Fish config change PATH depending on my architecture. Specifically I have two installations of homebrew, one for x86_64 and one for arm64. I want the brew command to pick the right one depending on what arch returns so I've got this in my config.fish:

if test (arch) = "i386"
  echo "Intel"
  set HOMEBREW_PREFIX /usr/local
else
  echo "M1"
  set HOMEBREW_PREFIX /opt/homebrew
end

fish_add_path $HOMEBREW_PREFIX/bin

Unfortunately the path gets messed up due to #31, and this PR fixes it.

I'm currently testing by doing this in my config.fish:

set fish_function_path $fish_function_path $HOME/git/chruby-fish/share/fish/vendor_functions.d

It's totally a hack, but I didn't want to make install until something more official happens with this PR.

Anyway, thanks for your time and I hope my context makes sense!

@ioquatix
Copy link
Collaborator

ioquatix commented Jan 6, 2022

@tenderlove just use my fork it merged all the patches.

@tenderlove
Copy link
Contributor

@tenderlove just use my fork it merged all the patches.

yep that's what I'm doing! 😄

@ioquatix
Copy link
Collaborator

ioquatix commented Jan 6, 2022

A subset of the engineers at my current work are using it with great success, so @JeanMertz it's well tested.

@ioquatix
Copy link
Collaborator

ioquatix commented Jan 6, 2022

@tenderlove feel free to submit a PR to the README on my repo if there is some interesting use case affecting M1, it's really helpful for people to document some "typical" use cases otherwise you can spend hours trying to figure out how to do something. Splitting M1/Rosetta is something a lot of Ruby engineers are wrangling with, so any insight you have would be most welcome.

@dalizard
Copy link

dalizard commented May 5, 2022

@JeanMertz Any capacity to take a look at those changes again? 😅

@JeanMertz
Copy link
Owner

Unfortunately, I don't use Ruby at my day job any more, but I'd be happy to add contributors to this repository to keep it maintained. If anyone in this discussion wants to take on that role, let me know.

@bouk
Copy link
Collaborator Author

bouk commented May 5, 2022

@JeanMertz I'll gladly take over maintainership of this project!

@JeanMertz
Copy link
Owner

@JeanMertz I'll gladly take over maintainership of this project!

Awesome. I sent you an invite.

@ioquatix
Copy link
Collaborator

ioquatix commented May 5, 2022

@JeanMertz can you please add me as an admin/maintainer too?

@JeanMertz
Copy link
Owner

@JeanMertz can you please add me as an admin/maintainer too?

Done. Between the two of you, I hope we can keep the project in a good state, to everyone's benefit. Thank you so much for making the effort to help everyone @bouk and @ioquatix ❤️

@bouk
Copy link
Collaborator Author

bouk commented May 6, 2022

Excellent, thank you @JeanMertz !

@JeanMertz JeanMertz dismissed their stale review May 6, 2022 08:33

outdated review

Currently not pinning to a specific Fish version.
@bouk
Copy link
Collaborator Author

bouk commented May 16, 2022

I've replaced the Travis config with GitHub Actions. @ioquatix this is now ready for review.

@JeanMertz could you disable Travis for this repo, or at least remove the required check? I can't access the settings.

@ioquatix
Copy link
Collaborator

I was largely happy with these changes and already have them in "production" so to speak, so I'm going to merge it and then follow up with any missing bits (if any) from my repo.

@ioquatix ioquatix merged commit fe2aec0 into JeanMertz:master May 17, 2022
@bouk bouk deleted the rewrite-fish branch May 17, 2022 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
10 participants